feat: add missing OpenAI/Claude/Gemini request fields#2971
feat: add missing OpenAI/Claude/Gemini request fields#2971seefs001 wants to merge 4 commits intoQuantumNous:mainfrom
Conversation
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
relay/common/override_test.go (1)
778-813: Add tests for the coreAllowIncludeObfuscationremoval logic.The new tests verify the pass-through skip paths, but there is no coverage for the actual
include_obfuscationremoval behaviour introduced inRemoveDisabledFields. The following scenarios are untested:
include_obfuscationis stripped whenAllowIncludeObfuscation = falseinclude_obfuscationis preserved whenAllowIncludeObfuscation = truestream_optionsis removed entirely wheninclude_obfuscationwas its only field (andAllowIncludeObfuscation = false)stream_optionsis kept (with its remaining fields) when other sibling fields (e.g.include_usage) co-exist🧪 Proposed additional test cases
+func TestRemoveDisabledFieldsRemovesIncludeObfuscationByDefault(t *testing.T) { + input := `{"stream_options":{"include_usage":true,"include_obfuscation":false}}` + settings := dto.ChannelOtherSettings{} // AllowIncludeObfuscation = false by default + + out, err := RemoveDisabledFields([]byte(input), settings, false) + if err != nil { + t.Fatalf("RemoveDisabledFields returned error: %v", err) + } + assertJSONEqual(t, `{"stream_options":{"include_usage":true}}`, string(out)) +} + +func TestRemoveDisabledFieldsRemovesStreamOptionsWhenOnlyIncludeObfuscation(t *testing.T) { + input := `{"model":"gpt-4","stream_options":{"include_obfuscation":false}}` + settings := dto.ChannelOtherSettings{} + + out, err := RemoveDisabledFields([]byte(input), settings, false) + if err != nil { + t.Fatalf("RemoveDisabledFields returned error: %v", err) + } + assertJSONEqual(t, `{"model":"gpt-4"}`, string(out)) +} + +func TestRemoveDisabledFieldsPreservesIncludeObfuscationWhenAllowed(t *testing.T) { + input := `{"stream_options":{"include_usage":true,"include_obfuscation":false}}` + settings := dto.ChannelOtherSettings{AllowIncludeObfuscation: true} + + out, err := RemoveDisabledFields([]byte(input), settings, false) + if err != nil { + t.Fatalf("RemoveDisabledFields returned error: %v", err) + } + assertJSONEqual(t, input, string(out)) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/common/override_test.go` around lines 778 - 813, Add unit tests covering RemoveDisabledFields's AllowIncludeObfuscation branch: write tests that call RemoveDisabledFields with dto.ChannelOtherSettings{AllowIncludeObfuscation: false} and payloads containing "stream_options":{"include_obfuscation":true} to assert include_obfuscation is removed and stream_options is removed if it becomes empty; another with AllowIncludeObfuscation: true to assert include_obfuscation is preserved; and a test where stream_options contains include_obfuscation plus another field (e.g. "include_usage") with AllowIncludeObfuscation:false to assert include_obfuscation is stripped but stream_options and the sibling field remain. Use function names like TestRemoveDisabledFields_StripsIncludeObfuscation_WhenNotAllowed, TestRemoveDisabledFields_PreservesIncludeObfuscation_WhenAllowed, and TestRemoveDisabledFields_RemovesEmptyStreamOptions to locate coverage for RemoveDisabledFields and dto.ChannelOtherSettings.relay/common/relay_info.go (1)
739-752: Redundant map re-assignment on line 748.
streamOptionsis the underlyingmap[string]interface{}obtained via type assertion fromdata["stream_options"]. Since Go maps are reference types,delete(streamOptions, "include_obfuscation")already mutates the map in-place, sodata["stream_options"]already reflects the deletion without any explicit re-assignment.♻️ Simplify the else branch
if len(streamOptions) == 0 { delete(data, "stream_options") - } else { - data["stream_options"] = streamOptions }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/common/relay_info.go` around lines 739 - 752, The code redundantly reassigns streamOptions back into data after mutating it; because maps are reference types in Go, deleting a key via delete(streamOptions, "include_obfuscation") already updates data["stream_options"]. Remove the unnecessary else branch that sets data["stream_options"] = streamOptions and instead only delete data["stream_options"] when streamOptions becomes empty; keep the existing type-assertion and delete(streamOptions, ...) logic inside the AllowIncludeObfuscation check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@relay/common/override_test.go`:
- Around line 778-813: Add unit tests covering RemoveDisabledFields's
AllowIncludeObfuscation branch: write tests that call RemoveDisabledFields with
dto.ChannelOtherSettings{AllowIncludeObfuscation: false} and payloads containing
"stream_options":{"include_obfuscation":true} to assert include_obfuscation is
removed and stream_options is removed if it becomes empty; another with
AllowIncludeObfuscation: true to assert include_obfuscation is preserved; and a
test where stream_options contains include_obfuscation plus another field (e.g.
"include_usage") with AllowIncludeObfuscation:false to assert
include_obfuscation is stripped but stream_options and the sibling field remain.
Use function names like
TestRemoveDisabledFields_StripsIncludeObfuscation_WhenNotAllowed,
TestRemoveDisabledFields_PreservesIncludeObfuscation_WhenAllowed, and
TestRemoveDisabledFields_RemovesEmptyStreamOptions to locate coverage for
RemoveDisabledFields and dto.ChannelOtherSettings.
In `@relay/common/relay_info.go`:
- Around line 739-752: The code redundantly reassigns streamOptions back into
data after mutating it; because maps are reference types in Go, deleting a key
via delete(streamOptions, "include_obfuscation") already updates
data["stream_options"]. Remove the unnecessary else branch that sets
data["stream_options"] = streamOptions and instead only delete
data["stream_options"] when streamOptions becomes empty; keep the existing
type-assertion and delete(streamOptions, ...) logic inside the
AllowIncludeObfuscation check.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
relay/common/override_test.go (1)
778-846: Add coverage forallow_include_obfuscationpassthrough.
You validate default removal and inference_geo, but not the “allowed” case for include_obfuscation. A small test will lock in the new behavior.✅ Suggested test
+func TestRemoveDisabledFieldsAllowIncludeObfuscation(t *testing.T) { + input := `{ + "stream_options":{"include_obfuscation":false}, + "store":true + }` + settings := dto.ChannelOtherSettings{ + AllowIncludeObfuscation: true, + } + + out, err := RemoveDisabledFields([]byte(input), settings, false) + if err != nil { + t.Fatalf("RemoveDisabledFields returned error: %v", err) + } + assertJSONEqual(t, input, string(out)) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/common/override_test.go` around lines 778 - 846, Add a unit test that verifies include_obfuscation is preserved when ChannelOtherSettings.AllowIncludeObfuscation is true: create a test (e.g. TestRemoveDisabledFieldsAllowIncludeObfuscation) that calls RemoveDisabledFields with input JSON containing "stream_options":{"include_obfuscation":false} (or true) and settings := dto.ChannelOtherSettings{AllowIncludeObfuscation: true}, assert no error and that the output JSON still contains the stream_options.include_obfuscation field using the existing assertJSONEqual helper; reference RemoveDisabledFields and dto.ChannelOtherSettings.AllowIncludeObfuscation to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@relay/common/override_test.go`:
- Around line 778-846: Add a unit test that verifies include_obfuscation is
preserved when ChannelOtherSettings.AllowIncludeObfuscation is true: create a
test (e.g. TestRemoveDisabledFieldsAllowIncludeObfuscation) that calls
RemoveDisabledFields with input JSON containing
"stream_options":{"include_obfuscation":false} (or true) and settings :=
dto.ChannelOtherSettings{AllowIncludeObfuscation: true}, assert no error and
that the output JSON still contains the stream_options.include_obfuscation field
using the existing assertJSONEqual helper; reference RemoveDisabledFields and
dto.ChannelOtherSettings.AllowIncludeObfuscation to locate where to add the
test.
Summary by CodeRabbit
New Features
Improvements
Tests